Skip to content

Conversation

@Dunsin-cyber
Copy link
Contributor

🚀 Summary

This pull request continues from where #3089 left off. That initial PR laid the groundwork, and this one builds upon it to complete the implementation and resolve remaining issues.

✨ Changes Introduced

A breakdown of what this PR introduces:

  • Fixed all failing tests, particularly for ConfirmPayment and SitePreferences.
  • ConfirmPayment Fix:
    Added a default mock implementation for getFormattedInCurrency in the SettingsContext. This resolves the failing unit tests in ConfirmPayment/index.test.tsx caused by a missing mock.
  • SitePreferences Fix:
    Work is still in progress for fixing the related test cases.

🔗 Related Issues or PRs

🧩 Type of Change

Select the appropriate type:

  • fix: Bug fix (non-breaking change which fixes an issue)
  • feat!: Breaking change (may break existing functionality)

📸 Screenshots (optional)

Add any relevant screenshots, screen recordings, or UI diffs to help reviewers.

🧪 How This Was Tested

  • Manual testing
  • Unit/integration tests
  • Test environment notes: [Optional: add environment or tool info if relevant]

✅ Checklist

  • Code reviewed and cleaned up
  • Manual tests run successfully
  • Automated tests added or updated
  • Documentation and guides updated (if applicable)

For UI-related changes:

  • Dark mode tested
  • Responsive layout verified

@Dunsin-cyber
Copy link
Contributor Author

hi @pavanjoshi914 , there were three test failing initaially, the recent push i made fixed two of them, so presently only one test is failing, and that's for SitePreferences, for ths particular test, i think the test is not really efficient and that is why it fails,

i noticed the test added for "set new budget" might be a bit too tightly coupled to the internal formatting behavior especially the exact number and order of calls to mockGetFormattedInCurrency as each digit is typed (2 → 25 → 250).

This makes the test a bit fragile and implementation-dependent, it could start failing if, for instance, the input handling is optimized (e.g. with debouncing or batching), even if the feature still behaves correctly for users.

It might be more robust to:

  • Assert that the formatting function is called with the final value (250) for both "BTC" and "USD".

  • Optionally verify a few key calls, but not every intermediate one.

  • Focus more on the final visible value or onEdit being triggered, rather than the total call count.

Interestingly, the initial version of the test (with mockGetFiatValue) already leaned in this direction. it focused more on verifying that the final value was passed and that the right callbacks were triggered, without overasserting internal details. It just needs updating to reflect the current DualCurrencyField implementation and the mockGetFormattedInCurrency function.

That way the test better reflects actual user-facing correctness without being overly strict on internals.

@Dunsin-cyber
Copy link
Contributor Author

i can rewrite the test to better illustrate what i’m suggesting if editing the test is welcomed.

@pavanjoshi914
Copy link
Contributor

yup if it makes sense

@Dunsin-cyber
Copy link
Contributor Author

yup if it makes sense

alright, i would do that.

@Dunsin-cyber
Copy link
Contributor Author

I refactored the effect that syncs the external value prop with the internal input state in DualCurrencyField to improve clarity, performance, and correctness. The previous logic was doing multiple conversions and comparisons that often led to redundant state updates and unnecessary re-renders. It also made the control flow harder to follow

so now, we track the last seen value and only run conversions when there's a meaningful change. I also ensure the input value is only updated if the final rendered value would actually change, avoiding unnecessary React updates.

@Dunsin-cyber Dunsin-cyber marked this pull request as ready for review June 26, 2025 21:55
@Dunsin-cyber Dunsin-cyber force-pushed the refine-dual-currency-3089 branch from 1877599 to 03216ea Compare July 1, 2025 11:54
@Dunsin-cyber
Copy link
Contributor Author

This PR got messy after a rebase/force-push — I’ve opened a clean version here: #3395

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit switching within the "Amount" input

3 participants